adds allowlist and denylist for headers #1178
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR introduces a global header filtering configuration feature that allows users to control which custom headers (x-bf-eh-* prefix) are forwarded to LLM providers via allowlist and denylist arrays. The configuration is persisted in the database, validated through HTTP handlers, and applied during request context conversion. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant Handler as HTTP Handler
participant ConfigStore as Config Store
participant DB as Database
participant Server as Server
participant Request as Request Handler
User->>Handler: POST /api/config with header_filter_config
rect rgb(200, 220, 255)
Note over Handler: Validation Phase
Handler->>Handler: validateHeaderFilterConfig()
alt Invalid (security headers detected)
Handler-->>User: HTTP 400 Bad Request
end
end
rect rgb(200, 255, 220)
Note over ConfigStore,DB: Persistence Phase
Handler->>ConfigStore: UpdateClientConfig(config)
ConfigStore->>ConfigStore: Serialize HeaderFilterConfig → JSON
ConfigStore->>DB: Store header_filter_config_json
DB-->>ConfigStore: Confirm
end
rect rgb(255, 240, 200)
Note over Server: Reload Phase
Handler->>Server: ReloadHeaderFilterConfig(config)
Server->>Server: Update in-memory ClientConfig.HeaderFilterConfig
Server-->>Handler: Success
end
Handler-->>User: HTTP 200 OK
rect rgb(255, 220, 220)
Note over Request: Subsequent Request (Header Filtering)
Request->>Request: ConvertToBifrostContext(ctx, headerFilterConfig)
alt Allowlist configured
Request->>Request: shouldAllowHeader() checks allowlist first
else Denylist configured
Request->>Request: shouldAllowHeader() filters by denylist
else No config
Request->>Request: Default security headers enforced
end
Request->>Request: Forward filtered headers to provider
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (39)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
transports/bifrost-http/lib/config.go (1)
39-41: Expose header filter via HandlerStore; consider future-proofing concurrent accessWiring
GetHeaderFilterConfigthroughHandlerStoreand implementing it onConfigby returningc.ClientConfig.HeaderFilterConfiggives the router/context code clean read-only access, and mirrors the existingShouldAllowDirectKeyspattern.If you expect frequent runtime reloads of the header filter config, it may be worth eventually backing
HeaderFilterConfigwith anatomic.Pointeror guarding writes with the existing mutex to keep the race detector quiet while still allowing cheap reads from hot paths. Not a blocker given the existing pattern, just something to consider.Also applies to: 2065-2071
transports/config.schema.json (1)
84-103: Clarify header_filter_config header name format (case and prefix).Runtime filtering (in
ConvertToBifrostContext) lowercases header names and strips thex-bf-eh-prefix before checking allowlist/denylist, but this schema’s descriptions don’t say that. If users configure"User-Agent"or include the prefix, those entries will never match the normalized"user-agent"label, and filtering will appear ineffective.Consider either:
- documenting explicitly that entries must be lowercase header names without the
x-bf-eh-prefix (e.g."user-agent"), or- normalizing the configured allowlist/denylist to lowercase at load time so configuration is case-insensitive.
This will avoid subtle misconfigurations and align user expectations with the actual filter behavior.
ui/lib/types/schemas.ts (1)
680-691: Consider normalizing and deduplicating header names in the Zod schemas.The schemas correctly mirror the backend shape, but you may want to harden UX here:
- Normalize to lowercase so config is case-insensitive and matches the runtime behavior that already lowercases header names before filtering.
- Optionally deduplicate entries to avoid redundant values.
For example:
export const globalHeaderFilterConfigSchema = z.object({ allowlist: z .array(z.string().min(1)) .transform(list => Array.from(new Set(list.map(h => h.toLowerCase())))) .optional(), denylist: z .array(z.string().min(1)) .transform(list => Array.from(new Set(list.map(h => h.toLowerCase())))) .optional(), });This keeps the API the same but makes misconfiguration much harder.
Also applies to: 709-710
transports/bifrost-http/handlers/inference.go (1)
449-451: Header filter wiring into ConvertToBifrostContext is correct but quite repetitive.All the updated call sites correctly pass
h.handlerStore.ShouldAllowDirectKeys()andh.config.GetHeaderFilterConfig()intoConvertToBifrostContext, so behavior-wise this looks good.Given how often this pattern appears, consider adding a small helper on
CompletionHandler, e.g.:func (h *CompletionHandler) bifrostContext(ctx *fasthttp.RequestCtx) (*context.Context, context.CancelFunc) { return lib.ConvertToBifrostContext(ctx, h.handlerStore.ShouldAllowDirectKeys(), h.config.GetHeaderFilterConfig()) }and then reusing it across handlers. That will make future changes to context creation (additional flags, config, etc.) much easier and less error-prone.
Also applies to: 573-574, 649-650, 731-732, 803-804, 879-880, 1010-1011, 1096-1097, 1395-1396, 1447-1449, 1487-1488, 1526-1527, 1565-1566, 1647-1648, 1706-1707, 1745-1746, 1784-1785, 1823-1824
transports/bifrost-http/handlers/config.go (1)
648-701: Consider deduplicating and filtering empty entries server-side.While the frontend filters empty strings before submission, the backend should also validate to ensure data integrity. Additionally, consider deduplicating entries in the allowlist/denylist to prevent user confusion.
🔎 Proposed enhancement for input sanitization
// Normalize header names to lowercase for case-insensitive matching + seen := make(map[string]bool) + var cleanedAllowlist []string for i, header := range payload.Allowlist { - payload.Allowlist[i] = strings.ToLower(strings.TrimSpace(header)) + normalized := strings.ToLower(strings.TrimSpace(header)) + if normalized != "" && !seen[normalized] { + cleanedAllowlist = append(cleanedAllowlist, normalized) + seen[normalized] = true + } } + payload.Allowlist = cleanedAllowlist + + seen = make(map[string]bool) + var cleanedDenylist []string for i, header := range payload.Denylist { - payload.Denylist[i] = strings.ToLower(strings.TrimSpace(header)) + normalized := strings.ToLower(strings.TrimSpace(header)) + if normalized != "" && !seen[normalized] { + cleanedDenylist = append(cleanedDenylist, normalized) + seen[normalized] = true + } } + payload.Denylist = cleanedDenylistui/app/workspace/config/views/headerFilterView.tsx (1)
30-47: Consider improving type safety for useFieldArray.The
as nevertype casts suggest a type mismatch between the form schema and useFieldArray expectations. This is a common pattern when using useFieldArray with primitive arrays (string[]) rather than object arrays.For better type safety, you could wrap the header strings in objects:
// In schema: allowlist: z.array(z.object({ value: z.string() })).optional() // In useFieldArray: name: "allowlist" // No cast neededHowever, since this works correctly at runtime and the current approach is common with react-hook-form, this is a minor improvement that can be deferred.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/header-filter/page.tsxui/app/workspace/config/views/headerFilterView.tsxui/components/sidebar.tsxui/lib/store/apis/baseApi.tsui/lib/store/apis/configApi.tsui/lib/types/config.tsui/lib/types/schemas.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/config/header-filter/page.tsxtransports/config.schema.jsonui/lib/types/config.tsui/lib/types/schemas.tstransports/bifrost-http/server/server.goui/components/sidebar.tsxtransports/bifrost-http/handlers/config.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/lib/ctx.goframework/configstore/clientconfig.goframework/configstore/store.goui/app/workspace/config/views/headerFilterView.tsxtransports/bifrost-http/handlers/inference.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/integrations/router.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/mcp.goframework/configstore/rdb.goui/lib/store/apis/configApi.tsui/lib/store/apis/baseApi.ts
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/lib/ctx.goframework/configstore/clientconfig.goframework/configstore/store.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/integrations/router.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/mcp.goframework/configstore/rdb.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/handlers/mcp.go
🧬 Code graph analysis (13)
ui/app/workspace/config/header-filter/page.tsx (1)
ui/app/workspace/config/views/headerFilterView.tsx (1)
HeaderFilterView(18-257)
ui/lib/types/config.ts (1)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)
transports/bifrost-http/server/server.go (3)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
transports/bifrost-http/handlers/config.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/integrations/bedrock_test.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
framework/configstore/clientconfig.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
framework/configstore/store.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
transports/bifrost-http/integrations/router.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
framework/configstore/tables/config.go (4)
core/providers/gemini/types.go (1)
Type(783-783)ui/lib/types/config.ts (1)
GlobalProxyType(269-269)core/network/http.go (1)
GlobalProxyType(46-46)core/schemas/provider.go (1)
NoProxy(147-147)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
ui/lib/store/apis/configApi.ts (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (27)
ui/lib/store/apis/baseApi.ts (1)
138-150: HeaderFilterConfig RTK Query tag wiring looks correctAdding
"HeaderFilterConfig"totagTypesis consistent with the new config endpoints and should allow proper cache invalidation with no behavioral regressions.ui/components/sidebar.tsx (1)
3-34: New “Header Filter” config nav entry is consistent and well-scopedImporting
Filterand adding the “Header Filter” sub-item under Config, gated byhasSettingsAccessand routed to/workspace/config/header-filter, aligns with existing sidebar patterns and RBAC usage.Also applies to: 532-538
transports/bifrost-http/lib/config.go (1)
385-388: HeaderFilterConfig load/merge and persistence pipeline is coherentThe way you:
- merge
HeaderFilterConfigwith DB-precedence inmergeClientConfig,- sync any file-/client-derived
HeaderFilterConfiginto the store inloadClientConfigFromFile, and- then hydrate
ClientConfig.HeaderFilterConfigfrom the store vialoadDefaultHeaderFilterConfig(for both file-backed and default paths)keeps a single source of truth in the config store while still supporting initial config.json defaults and the “no store” mode. This matches how other config sections are handled.
Also applies to: 501-507, 548-559, 1578-1580, 1763-1773
transports/bifrost-http/integrations/bedrock_test.go (1)
9-10: mockHandlerStore correctly updated for new HandlerStore contractExtending
mockHandlerStorewith aheaderFilterConfigfield andGetHeaderFilterConfigimplementation, plus the added tables import, cleanly aligns the tests with the newlib.HandlerStoreinterface without changing existing test behavior (nil config → default filtering).Also applies to: 17-19, 25-27
transports/bifrost-http/integrations/router.go (1)
392-399: Router now passes headerFilterConfig into ConvertToBifrostContext as intendedUsing
g.handlerStore.GetHeaderFilterConfig()alongsideShouldAllowDirectKeys()when callingConvertToBifrostContextcleanly threads the global header filtering rules into every integration route without touching downstream handler logic.ui/app/workspace/config/header-filter/page.tsx (1)
1-11: HeaderFilterPage wiring is minimal and consistent with other config pagesClient-side
HeaderFilterPagesimply wrapsHeaderFilterViewin the usualmax-w-7xllayout and lives at the path referenced by the new sidebar item, which is exactly what’s needed here.framework/configstore/clientconfig.go (1)
37-51: ClientConfig field and hash integration for HeaderFilterConfig look consistent.The new
HeaderFilterConfigfield and its inclusion inGenerateClientConfigHashare wired correctly: allowlist/denylist are sorted before hashing for determinism, and nil vs empty lists intentionally have no hash impact, matching their identical runtime behavior.No changes needed here.
Also applies to: 144-170
transports/bifrost-http/handlers/mcp.go (1)
53-73: MCP tool handler correctly adopts header filter–aware context.Using
ConvertToBifrostContext(ctx, false, h.store.GetHeaderFilterConfig())keeps MCP requests opt-out of direct keys while still applying the global header filter configuration. This is consistent with the rest of the stack and doesn’t introduce new behavior quirks.Looks good as-is.
framework/configstore/store.go (1)
122-129: ConfigStore interface extension for header filter config is consistent; ensure all impls are updated.Adding
GetHeaderFilterConfig/UpdateHeaderFilterConfighere fits the existing pattern for global config (proxy, restart, auth, etc.) and matches thetables.GlobalHeaderFilterConfigtype.Just make sure every concrete
ConfigStoreimplementation and any mocks (e.g., sqlite/postgres stores, in-memory test doubles) implement these methods so the stack compiles and tests continue to pass.ui/lib/types/config.ts (1)
302-313: TS config types for global header filter match backend and defaults correctly.
GlobalHeaderFilterConfigandDefaultGlobalHeaderFilterConfigalign withtables.GlobalHeaderFilterConfig, and wiring it intoBifrostConfigviaheader_filter_config?: GlobalHeaderFilterConfigmaintains backward compatibility (field is optional) while exposing the new capability to the UI.No changes needed here.
Also applies to: 321-329
framework/configstore/rdb.go (1)
1952-1981: LGTM! Methods follow established patterns.The
GetHeaderFilterConfigandUpdateHeaderFilterConfigmethods are consistent with existing governance config methods likeGetProxyConfigandGetRestartRequiredConfig. The error handling, nil checks, and JSON marshalling patterns align well with the codebase conventions.framework/configstore/tables/config.go (2)
40-49: LGTM! Well-documented struct with clear filter logic.The
GlobalHeaderFilterConfigstruct is well-designed with clear documentation explaining the filter precedence logic. The JSON tags withomitemptyallow for flexible partial configuration.
12-12: LGTM!The constant follows the established naming convention for governance config keys.
transports/bifrost-http/server/server.go (2)
841-856: LGTM! Clean hot-reload implementation.The method correctly stores the header filter config in memory and provides useful debug logging. Unlike proxy configuration, header filter changes take effect immediately without requiring a restart, which is appropriate since the filter is applied per-request.
66-66: LGTM!The interface extension is consistent with the existing callback pattern.
ui/lib/store/apis/configApi.ts (2)
90-106: LGTM! RTK Query endpoints follow established patterns.The new endpoints correctly mirror the existing proxy config pattern. The cache invalidation strategy invalidating both
HeaderFilterConfigandConfigtags ensures the UI stays synchronized when header filter settings change.
124-125: The hook nameuseUpdateHeaderFilterConfigMutationis correct and consistent throughout the codebase. No typo exists.Likely an incorrect or invalid review comment.
transports/bifrost-http/handlers/config.go (2)
626-645: LGTM! Handler follows established patterns.The
getHeaderFilterConfighandler correctly returns empty arrays when config is nil, providing a consistent response structure for the frontend.
163-169: LGTM!Header filter config is correctly included in the aggregated config response, consistent with how proxy config is handled.
ui/app/workspace/config/views/headerFilterView.tsx (4)
49-59: LGTM! Proper form synchronization with fetched data.The useEffect correctly resets the form when server data is fetched, ensuring arrays are properly initialized even if the server returns undefined values.
61-73: LGTM! Clean data sanitization before submission.The onSubmit handler correctly filters out empty strings before sending to the API, preventing invalid entries from being persisted.
114-126: LGTM! Clear and informative filter precedence documentation.The alert clearly explains the filter logic to users, which matches the implementation in the backend. This is excellent UX design.
239-251: LGTM! Security transparency.Displaying the list of always-blocked headers provides good transparency to users about the security measures in place.
transports/bifrost-http/lib/ctx.go (4)
100-116: LGTM! Security denylist is comprehensive.The security denylist correctly blocks sensitive headers that could be exploited:
- Authentication headers (authorization, proxy-authorization, cookie)
- Connection headers (host, content-length, connection, transfer-encoding)
- API key headers that could override governance controls
118-145: LGTM! Filter logic implementation is correct.The
shouldAllowHeaderfunction correctly implements the documented precedence:
- Nil config → allow all
- Allowlist check (if present) → must be in list
- Denylist check (if present) → must not be in list
- Both empty → allow all
The implementation matches the documentation in
GlobalHeaderFilterConfigand the UI explanation.
287-294: LGTM! Filter application is in the correct order.The security denylist is checked first (always enforced), followed by the configurable filter. This ensures security-sensitive headers cannot be forwarded even if mistakenly added to an allowlist.
80-80: All callers have been properly updated. All 21 call sites across router.go, mcp.go, and inference.go pass the required three arguments: context, allowDirectKeys flag, and headerFilterConfig.
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
ba75398 to
a65444e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ui/app/workspace/config/views/clientSettingsView.tsx (3)
30-45: Consider order-insensitive comparison for header filter config.The current comparison is order-sensitive, meaning
["a", "b"]and["b", "a"]are considered different. While this works for tracking local edits, it could cause false "unsaved changes" detection if the server returns headers in a different order than stored locally.🔎 Proposed order-insensitive comparison
function headerFilterConfigEqual (a?: GlobalHeaderFilterConfig, b?: GlobalHeaderFilterConfig): boolean { const aAllowlist = a?.allowlist || [] const bAllowlist = b?.allowlist || [] const aDenylist = a?.denylist || [] const bDenylist = b?.denylist || [] if (aAllowlist.length !== bAllowlist.length || aDenylist.length !== bDenylist.length) { return false } + const sortedAAllow = [...aAllowlist].sort() + const sortedBAllow = [...bAllowlist].sort() + const sortedADeny = [...aDenylist].sort() + const sortedBDeny = [...bDenylist].sort() + return ( - aAllowlist.every((v, i) => v === bAllowlist[i]) && - aDenylist.every((v, i) => v === bDenylist[i]) + sortedAAllow.every((v, i) => v === sortedBAllow[i]) && + sortedADeny.every((v, i) => v === sortedBDeny[i]) ) }
84-99: Consider adding a defensive check before accessingbifrostConfig.The non-null assertion
bifrostConfig!on line 94 is technically safe due to the button's disabled logic, but an explicit guard makes the code more robust against future refactoring.🔎 Proposed defensive check
const handleSave = useCallback(async () => { + if (!bifrostConfig) return try { // Clean up empty strings from header filter config const cleanedConfig = { ...localConfig, header_filter_config: { allowlist: (localConfig.header_filter_config?.allowlist || []).filter((h) => h && h.trim().length > 0), denylist: (localConfig.header_filter_config?.denylist || []).filter((h) => h && h.trim().length > 0), }, } - await updateCoreConfig({ ...bifrostConfig!, client_config: cleanedConfig }).unwrap() + await updateCoreConfig({ ...bifrostConfig, client_config: cleanedConfig }).unwrap() toast.success("Client settings updated successfully.") } catch (error) { toast.error(getErrorMessage(error)) } }, [bifrostConfig, localConfig, updateCoreConfig])
258-278: Usingindexaskeyfor dynamic lists may cause subtle issues.When items are removed from the middle of the list, React may preserve DOM state incorrectly because keys shift. For controlled inputs this usually works, but could cause issues with focus or animations. Consider using a stable identifier if problems arise.
One approach would be to generate unique IDs when adding headers:
// Instead of storing string[], store { id: string, value: string }[] // Then use key={item.id} in the mapFor now, since these are simple controlled inputs and the list is typically small, this is acceptable.
Also applies to: 302-322
transports/bifrost-http/handlers/config.go (1)
25-35: Header filter config wiring is correct; consider order-insensitive equalityThe new pieces here hang together well:
ConfigManagergainsReloadHeaderFilterConfig, andupdateConfiginvokes it only when the client’sHeaderFilterConfigactually differs.getConfigexposesheader_filter_configfrom the store alongside existing config sections, which is what the UI and API need.One behavioral nuance:
headerFilterConfigEqualusesslices.Equaldirectly, so allowlist/denylist comparisons are order‑sensitive and treatnilvs non‑nilGlobalHeaderFilterConfigas different even when both lists are empty. Since the filter semantics are set‑like (order doesn’t matter, and “no headers configured” vs “empty lists” behave the same inConvertToBifrostContext), you might want to normalize before comparison (e.g., sort and/or treat empty configs as equivalent) to avoid unnecessary reloads when only list ordering changes.If you’re fine with “any structural difference triggers a reload,” the current implementation is otherwise sound.
Also applies to: 161-167, 176-176, 188-192, 223-225, 276-285, 633-642
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- framework/configstore/tables/config.go
- framework/configstore/rdb.go
- transports/bifrost-http/lib/config.go
- ui/lib/types/config.ts
- transports/bifrost-http/integrations/bedrock_test.go
- transports/config.schema.json
- ui/lib/types/schemas.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/configstore/clientconfig.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.goui/app/workspace/config/views/clientSettingsView.tsxframework/configstore/store.gotransports/bifrost-http/server/server.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
framework/configstore/clientconfig.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.goframework/configstore/store.gotransports/bifrost-http/server/server.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/server/server.go
🧬 Code graph analysis (7)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
transports/bifrost-http/handlers/config.go (3)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
transports/bifrost-http/integrations/router.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
ui/app/workspace/config/views/clientSettingsView.tsx (4)
ui/lib/types/config.ts (3)
CoreConfig(336-350)DefaultGlobalHeaderFilterConfig(310-313)GlobalHeaderFilterConfig(304-307)ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(63-65)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(174-199)ui/components/ui/input.tsx (1)
Input(15-69)
framework/configstore/store.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (14)
ui/app/workspace/config/views/clientSettingsView.tsx (3)
101-164: LGTM!The header filter list handlers are well-structured with proper memoization via
useCallback. The lowercase normalization on input change aligns with the security requirements mentioned in the PR objectives.
166-221: LGTM!The component layout and toggle sections are well-structured. RBAC checks are consistently applied to disable controls when the user lacks update permissions. The conditional display of dropped request count is a nice UX touch.
223-349: LGTM!The header forwarding section provides excellent documentation through the Alert components, clearly explaining the allowlist/denylist behavior and the always-blocked security headers. The UI flows for managing headers are intuitive.
framework/configstore/clientconfig.go (2)
37-50: ClientConfig: header filter field wiring looks consistentAdding
HeaderFilterConfig *tables.GlobalHeaderFilterConfigwithomitemptyand a clear comment fits the existing config surface and keeps it optional without breaking JSON shape.
144-170: HeaderFilterConfig hashing is deterministic and aligned with semanticsIncluding sorted allowlist/denylist in
GenerateClientConfigHash(with explicit prefixes) makes hash changes stable and order‑insensitive, matching how other slice fields are handled and what the filter logic expects.transports/bifrost-http/handlers/inference.go (1)
450-451: Consistent propagation of header filter config into all inference/file handlersPassing
h.config.GetHeaderFilterConfig()intoConvertToBifrostContextfor every handler keeps header forwarding rules centralized and guarantees that the allowlist/denylist is respected across all Bifrost HTTP endpoints without altering existing cancellation/error behavior.Also applies to: 573-574, 649-650, 731-732, 803-805, 879-880, 1010-1011, 1096-1097, 1395-1397, 1448-1449, 1487-1488, 1526-1527, 1565-1566, 1647-1648, 1706-1707, 1745-1746, 1784-1785, 1823-1824
transports/bifrost-http/integrations/router.go (1)
393-400: Integration router now correctly applies global header filter configUsing
g.handlerStore.GetHeaderFilterConfig()when buildingbifrostCtxaligns integrations with the core handlers so that x‑bf‑eh‑* forwarding is governed by the same allowlist/denylist rules on all routes.transports/bifrost-http/handlers/mcp.go (1)
67-68: MCP execution correctly opts into header filteringPassing
h.store.GetHeaderFilterConfig()intoConvertToBifrostContextfor MCP tool execution keeps header behavior consistent with the rest of the transport while still disallowing direct keys for this endpoint.framework/configstore/store.go (1)
126-129: ConfigStore extension for header filter config is well-scopedAdding dedicated
GetHeaderFilterConfig/UpdateHeaderFilterConfigmethods mirrors existing global config CRUD patterns (e.g., proxy, restart_required) and gives a clear persistence API forGlobalHeaderFilterConfig.transports/bifrost-http/server/server.go (1)
53-67: Server-side reload hook for header filter config is straightforward and sufficientExtending
ServerCallbacksand implementingReloadHeaderFilterConfigby updatings.Config.ClientConfig.HeaderFilterConfigand logging allowlist/denylist sizes gives a clear, low‑friction way to hot‑apply header filter changes without restart and fits the existing Reload*Config patterns in this server.Also applies to: 841-856
transports/bifrost-http/lib/ctx.go (4)
11-11: LGTM!Imports are appropriate for the new header filtering functionality.
Also applies to: 18-18
80-80: LGTM!The function signature extension is clean. Using a pointer allows
nilto represent "no filtering config" with graceful handling inside the function.
100-116: LGTM!Good rename to
securityDenylistto clearly distinguish the always-enforced security list from the user-configurable denylist. The coverage of security-sensitive headers (auth, proxy-auth, cookies, connection headers, and key-override headers) is comprehensive.
287-294: LGTM!Correct enforcement order: security denylist is checked first (always enforced), then the configurable filter is applied. This ensures security-sensitive headers are never forwarded regardless of user configuration.
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
a65444e to
312c22a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
transports/bifrost-http/lib/ctx.go (1)
118-145: Header filter allow/deny checks are case-sensitive despite lowercasing header names.
labelNameis normalized to lowercase before callingshouldAllowHeader, but the allow/deny checks useslices.Containsagainst the rawAllowlist/Denylist, which are not normalized. A config entry like"Content-Type"will not match the incoming"content-type", so filters behave unexpectedly and contradict the stated case‑insensitive semantics.Recommend making membership checks case‑insensitive (or normalizing config to lowercase on load). One local fix is:
Proposed fix: use case-insensitive membership via
strings.EqualFold- hasAllowlist := len(headerFilterConfig.Allowlist) > 0 - hasDenylist := len(headerFilterConfig.Denylist) > 0 + allowlist := headerFilterConfig.Allowlist + denylist := headerFilterConfig.Denylist + hasAllowlist := len(allowlist) > 0 + hasDenylist := len(denylist) > 0 @@ - if hasAllowlist { - if !slices.Contains(headerFilterConfig.Allowlist, headerName) { + if hasAllowlist { + if !slices.ContainsFunc(allowlist, func(s string) bool { + return strings.EqualFold(s, headerName) + }) { return false } } // If denylist is non-empty, header must not be in denylist - if hasDenylist { - if slices.Contains(headerFilterConfig.Denylist, headerName) { + if hasDenylist { + if slices.ContainsFunc(denylist, func(s string) bool { + return strings.EqualFold(s, headerName) + }) { return false } }This keeps header matching truly case-insensitive while leaving config storage format unchanged.
🧹 Nitpick comments (5)
ui/lib/types/schemas.ts (1)
680-691: Global header filter schemas/types line up with backend; optional normalization.The Zod schemas and inferred types correctly mirror the backend
GlobalHeaderFilterConfig(allowlist/denylist) and theheader_filter_configwrapper shape, so they should interop cleanly with/api/config. If header names are meant to be case-insensitive, you may optionally add a.transform()(e.g.,s.trim().toLowerCase()) on the string items here so UI input is canonicalized before it hits the backend filters.Also applies to: 709-710
transports/bifrost-http/handlers/config.go (1)
34-35: Header filter config wiring is sound; verify persistence and equality semantics.
- Extending
ConfigManagerwithReloadHeaderFilterConfigand calling it fromupdateConfigwhenHeaderFilterConfigchanges is a good way to hot‑reload filter rules without a restart.getConfigexposingheader_filter_configviaConfigStore.GetHeaderFilterConfiggives the UI a clean, focused payload in addition to whatever is onclient_config.headerFilterConfigEqual’s pointer/len-based comparison (treatingnilvs empty slices as equal and being order-sensitive) is reasonable; just be aware that reordering entries alone will trigger a reload.The only thing that’s not obvious from this file is whether
HeaderFilterConfigchanges are persisted to the same storage that backsGetHeaderFilterConfig(e.g., viaUpdateHeaderFilterConfigor insideUpdateClientConfig). If that linkage isn’t already in place inconfigstore, consider adding it so that:
GET /api/configalways returns the latest header filter config, and- the configuration survives process restarts.
Also applies to: 161-167, 276-285, 633-642
transports/bifrost-http/lib/config.go (1)
36-41: Header filter config bootstrapping and HandlerStore exposure look correct.
- Extending
HandlerStorewithGetHeaderFilterConfigand implementing it onConfigas a lock-free accessor overClientConfig.HeaderFilterConfigis consistent with the existingShouldAllowDirectKeyspattern.- Both config load paths (file-backed and default/SQLite) now call
loadDefaultHeaderFilterConfig, so runtime always picks up whateverConfigStore.GetHeaderFilterConfigreturns.loadClientConfigFromFilesyncingHeaderFilterConfiginto the store andmergeClientConfig’s “DB wins, fill missing allow/deny lists from file” behavior give you a sane single source of truth while still honoring file defaults.If you later decide to canonicalize header names (e.g., trimming and lowercasing allowlist/denylist entries once up front),
loadDefaultHeaderFilterConfigand the sync block inloadClientConfigFromFileare good central places to do that so all consumers see normalized data.Also applies to: 355-373, 501-506, 548-559, 1578-1580, 1763-1773, 2065-2071
ui/app/workspace/config/views/clientSettingsView.tsx (2)
280-300: Using array index askeycan cause input focus and value issues when items are removed.When items are added or removed from the middle of the list, React's reconciliation may not correctly associate inputs with their values, potentially causing the cursor to jump or values to appear in wrong inputs.
Consider generating a stable unique ID when adding items, or using a combination approach.
🔎 Proposed approach using stable IDs
One approach is to store objects with IDs instead of plain strings:
// In state, use objects with stable IDs: interface HeaderEntry { id: string; value: string; } // When adding: const handleAddAllowlistHeader = useCallback(() => { setLocalConfig((prev) => ({ ...prev, header_filter_config: { ...prev.header_filter_config, allowlist: [...(prev.header_filter_config?.allowlist || []), { id: crypto.randomUUID(), value: "" }], }, })); }, []);Alternatively, if keeping plain strings, you could use a composite key, though this is less robust:
key={`${header}-${index}`}
316-336: Samekey={index}concern applies to denylist.As noted for the allowlist above, using index as key here has the same potential for input confusion when items are removed. The same mitigation approach would apply.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- transports/bifrost-http/integrations/bedrock_test.go
- transports/config.schema.json
- framework/configstore/store.go
- framework/configstore/clientconfig.go
- framework/configstore/tables/config.go
- ui/lib/types/config.ts
- transports/bifrost-http/handlers/inference.go
- framework/configstore/rdb.go
- transports/bifrost-http/integrations/router.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.goui/app/workspace/config/views/clientSettingsView.tsxtransports/bifrost-http/handlers/config.goui/lib/types/schemas.tstransports/bifrost-http/lib/config.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.go
🧬 Code graph analysis (4)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
ui/app/workspace/config/views/clientSettingsView.tsx (3)
ui/lib/types/config.ts (3)
DefaultGlobalHeaderFilterConfig(310-313)GlobalHeaderFilterConfig(304-307)CoreConfig(336-350)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(174-199)ui/components/ui/input.tsx (1)
Input(15-69)
transports/bifrost-http/handlers/config.go (4)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (6)
transports/bifrost-http/handlers/mcp.go (1)
67-68: MCP execute path correctly participates in header filtering.Passing
h.store.GetHeaderFilterConfig()intoConvertToBifrostContextaligns MCP tool execution with the inference/router paths and ensures extra headers here respect the same filter rules.transports/bifrost-http/server/server.go (1)
66-67: Server-side header filter reload is correct; confirm persistence path.The new
ReloadHeaderFilterConfigcallback cleanly updates the in-memoryClientConfig.HeaderFilterConfigand logs a concise summary, which is enough for live header filtering to see changes.Since this method is intentionally in-memory only, please confirm that the handler/configstore path (e.g.,
UpdateClientConfigand/orUpdateHeaderFilterConfig) also persists the same config so that:
GetHeaderFilterConfigused byGET /api/configreturns the updated version, and- the filter configuration survives process restarts.
Also applies to: 841-856
ui/app/workspace/config/views/clientSettingsView.tsx (4)
30-42: Order-sensitive comparison may not detect reordered headers as "unchanged".The
headerFilterConfigEqualfunction compares arrays element-by-element by index. If users manually reorder headers externally or via future drag-and-drop functionality, identical sets in different orders would be flagged as changed.For the current UI (which doesn't support reordering), this works correctly. If reordering is added later, consider using a Set-based comparison.
81-96: Good sanitization of empty entries before save.The cleanup logic correctly filters out empty and whitespace-only strings from both allowlist and denylist before persisting. This prevents invalid entries from being saved.
223-270: Clear and helpful documentation for users.The accordion sections provide good explanations of how the allowlist/denylist precedence works and which headers are always blocked for security. This helps users understand the filtering behavior without needing external documentation.
119-127: Lowercase normalization applied correctly on input change.Both
handleAllowlistChangeandhandleDenylistChangecorrectly normalize header names to lowercase, ensuring case-insensitive matching as specified in the PR objectives.Also applies to: 149-157
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
312c22a to
dcb3c25
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
transports/bifrost-http/lib/ctx.go (1)
80-85: Header filter checks are still case‑sensitive against lowercased header names.Incoming headers are normalized to lowercase (
keyStr,labelName), but bothshouldAllowHeaderand the direct forwarding block compare againstheaderFilterConfig.Allowlist/Denylistusingslices.Contains, which is case‑sensitive. The UI now lowercases entries, but configs set via JSON or direct API can still use mixed‑case names (e.g.,Content-Type), which won’t match the lowercased header keys and will silently bypass the filter logic.To make matching truly case‑insensitive and robust across all writers, consider switching these membership tests to something like
slices.ContainsFunc(list, func(s string) bool { return strings.EqualFold(s, headerName) })(and the analogous check in the direct forwarding branch), or factoring out a helper that doesEqualFoldfor both allowlist and denylist.Also applies to: 100-115, 124-144, 279-297, 298-321
🧹 Nitpick comments (3)
transports/bifrost-http/handlers/config.go (1)
632-642: Consider:slices.Equalis order-sensitive.The comparison uses
slices.Equalwhich is order-sensitive. If two configs have the same headers in different order (e.g.,["a", "b"]vs["b", "a"]), they will be considered different, triggering unnecessary reloads.If header order is semantically irrelevant, consider sorting before comparison or using a set-based comparison.
🔎 Optional: Order-insensitive comparison
+import "sort" + +// slicesEqualUnordered compares two string slices ignoring order +func slicesEqualUnordered(a, b []string) bool { + if len(a) != len(b) { + return false + } + aCopy := make([]string, len(a)) + bCopy := make([]string, len(b)) + copy(aCopy, a) + copy(bCopy, b) + sort.Strings(aCopy) + sort.Strings(bCopy) + return slices.Equal(aCopy, bCopy) +} func headerFilterConfigEqual(a, b *configstoreTables.GlobalHeaderFilterConfig) bool { if a == nil && b == nil { return true } if a == nil || b == nil { return false } - return slices.Equal(a.Allowlist, b.Allowlist) && slices.Equal(a.Denylist, b.Denylist) + return slicesEqualUnordered(a.Allowlist, b.Allowlist) && slicesEqualUnordered(a.Denylist, b.Denylist) }transports/bifrost-http/lib/config.go (1)
37-41: Header filter plumbing looks coherent; consider normalizing config values once at load.The HandlerStore extension and the end‑to‑end wiring (client config merge, store sync via
UpdateHeaderFilterConfig, default‐path load, andGetHeaderFilterConfigaccessor) are consistent and match the intended “store as source of truth, file as seed” behavior; semantics for nil vs empty allow/deny lists also align with the runtime logic inctx.go.As a small robustness improvement, you could normalize header filter entries (e.g.,
strings.TrimSpace+strings.ToLower) once when merging/loading intoClientConfig.HeaderFilterConfig, so non‑UI writers (config file / direct API) can safely rely on case‑insensitive matching without having to follow the UI’s lowercase convention; this would keepctx.gosimple while hardening behavior against stray whitespace/case in persisted configs.Also applies to: 385-388, 501-506, 548-559, 1578-1580, 1763-1773, 2065-2071
ui/app/workspace/config/views/clientSettingsView.tsx (1)
27-42: UI wiring for header forwarding looks good; trim+lowercase values to avoid subtle mismatches.The defaulting of
header_filter_config, equality helper, and the allowlist/denylist UI all line up well with the backend, and forcingvalue.toLowerCase()ensures the lists stay consistent with lowercased header keys inctx.go. One small robustness tweak: consider normalizing withvalue.trim().toLowerCase()inhandleAllowlistChange/handleDenylistChange(and optionally serializing the trimmed version inhandleSave) so that stray leading/trailing spaces don’t result in entries like" custom-id"that will never match any real header name.Also applies to: 59-65, 81-91, 98-157, 295-371
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/config.schema.json
- framework/configstore/store.go
- transports/bifrost-http/handlers/inference.go
- framework/configstore/clientconfig.go
- framework/configstore/tables/config.go
- ui/lib/types/schemas.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/handlers/config.goui/lib/types/config.tstransports/bifrost-http/lib/ctx.goframework/configstore/rdb.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.goui/app/workspace/config/views/clientSettingsView.tsxtransports/bifrost-http/integrations/router.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.goframework/configstore/rdb.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/router.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/router.go
🧬 Code graph analysis (10)
transports/bifrost-http/integrations/bedrock_test.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/handlers/config.go (4)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)
ui/lib/types/config.ts (1)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
framework/configstore/rdb.go (2)
framework/configstore/tables/config.go (4)
GlobalHeaderFilterConfig(46-49)TableGovernanceConfig(52-55)TableGovernanceConfig(58-58)ConfigHeaderFilterKey(12-12)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/lib/config.go (3)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
transports/bifrost-http/server/server.go (4)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)transports/bifrost-http/lib/config.go (1)
Config(186-217)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
ui/app/workspace/config/views/clientSettingsView.tsx (2)
ui/lib/types/config.ts (3)
DefaultGlobalHeaderFilterConfig(310-313)GlobalHeaderFilterConfig(304-307)CoreConfig(336-350)framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)
transports/bifrost-http/integrations/router.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (12)
transports/bifrost-http/handlers/mcp.go (1)
67-67: LGTM!The header filter configuration is correctly passed to
ConvertToBifrostContext, aligning with the updated signature and ensuring MCP tool execution respects header filtering rules.transports/bifrost-http/integrations/bedrock_test.go (1)
15-30: LGTM!The mock correctly implements
GetHeaderFilterConfig()to satisfy the updatedlib.HandlerStoreinterface. The compile-time interface assertion at line 30 ensures the mock stays synchronized with interface changes.transports/bifrost-http/handlers/config.go (2)
161-167: LGTM!Header filter config is correctly fetched from the store and included in the response when available. The nil check ensures no empty entry is added to the response map.
276-284: LGTM!The change detection and reload flow is well-structured. The early return on reload failure ensures partial updates don't persist, maintaining configuration consistency.
ui/lib/types/config.ts (3)
302-313: LGTM!The
GlobalHeaderFilterConfiginterface and default constant are correctly defined and align with the Go backend struct atframework/configstore/tables/config.go. The optional array types match theomitemptyJSON tags in Go.
327-327: LGTM!The
header_filter_configfield is correctly added toBifrostConfig, enabling the UI to receive and display the header filter configuration from the API response.
349-349: LGTM!The
header_filter_configfield inCoreConfigmatches the GoClientConfigstruct, ensuring type consistency for configuration updates.framework/configstore/rdb.go (2)
1952-1969: LGTM!The
GetHeaderFilterConfigimplementation follows the established pattern used byGetProxyConfigand other governance config retrieval methods. The nil handling for both missing records and empty values is appropriate.
1971-1981: LGTM!The
UpdateHeaderFilterConfigimplementation is straightforward and consistent withUpdateProxyConfig. UsingSavewith the governance config key ensures proper upsert behavior.transports/bifrost-http/server/server.go (2)
66-66: LGTM!The
ReloadHeaderFilterConfigmethod is correctly added to theServerCallbacksinterface, enabling the config handler to trigger runtime updates.
841-856: LGTM!The implementation correctly stores the header filter config in
ClientConfig.HeaderFilterConfigand includes defensive nil checks before accessing slice lengths. The logging provides useful visibility into the configuration state.The pattern matches
ReloadProxyConfigabove, maintaining consistency across runtime configuration updates.transports/bifrost-http/integrations/router.go (1)
383-399: Passing header filter config into ConvertToBifrostContext is consistent and minimal.The updated call cleanly threads
GetHeaderFilterConfig()into context creation without affecting the existing request/streaming pipeline, and lines up with the new HandlerStore contract inlib/config.go.
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
dcb3c25 to
d1b85e4
Compare
akshaydeo
force-pushed
the
12-26-minor_ux_uplifg_restart_required_status_storage
branch
3 times, most recently
from
4f454ea to
6bcbc30
Compare
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
d1b85e4 to
11fcf39
Compare
akshaydeo
changed the base branch from
12-26-minor_ux_uplifg_restart_required_status_storage
to
graphite-base/1178
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
11fcf39 to
69645a0
Compare
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
69645a0 to
8a88ccf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/config.go (1)
223-225: Header filter config reload is not clearly persisted and equality is order-sensitiveTwo points to address:
Persistence gap (likely bug)
getConfigreads the header filter viaConfigStore.GetHeaderFilterConfig(ctx)and exposes it as top-levelheader_filter_config.updateConfigupdatesupdatedConfig.HeaderFilterConfigand callsconfigManager.ReloadHeaderFilterConfig, but never calls a store-levelUpdateHeaderFilterConfig.- Unless
ReloadHeaderFilterConfigitself writes through to the config store, this means header filter changes coming from/api/configwill not be persisted in the governance store, and may:
- Not show up in subsequent
/api/config?from_db=truereads, and- Be lost on server restart.
Consider wiring persistence explicitly here, e.g.:
if !headerFilterConfigEqual(payload.ClientConfig.HeaderFilterConfig, currentConfig.HeaderFilterConfig) { updatedConfig.HeaderFilterConfig = payload.ClientConfig.HeaderFilterConfig if err := h.store.ConfigStore.UpdateHeaderFilterConfig(ctx, payload.ClientConfig.HeaderFilterConfig); err != nil { logger.Warn(fmt.Sprintf("failed to save header filter config: %v", err)) SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to save header filter config: %v", err)) return } if err := h.configManager.ReloadHeaderFilterConfig(ctx, payload.ClientConfig.HeaderFilterConfig); err != nil { logger.Warn(fmt.Sprintf("failed to reload header filter config: %v", err)) SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to reload header filter config: %v", err)) return } }Or, if persistence is intentionally delegated to
ReloadHeaderFilterConfig, document that and ensure all implementations write through consistently.Order-sensitive equality
headerFilterConfigEqualusesslices.EqualonAllowlistandDenylist, so[ "a", "b" ]differs from[ "b", "a" ]. That means harmless reordering in the UI will trigger reloads and DB writes.- If you intend allow/deny lists to behave as sets, sort copies before comparing or compare via map/set.
For example:
func headerFilterConfigEqual(a, b *configstoreTables.GlobalHeaderFilterConfig) bool { if a == nil && b == nil { return true } if a == nil || b == nil { return false } al := slices.Clone(a.Allowlist) bl := slices.Clone(b.Allowlist) ad := slices.Clone(a.Denylist) bd := slices.Clone(b.Denylist) slices.Sort(al); slices.Sort(bl) slices.Sort(ad); slices.Sort(bd) return slices.Equal(al, bl) && slices.Equal(ad, bd) }Also applies to: 276-285, 295-301, 633-642
♻️ Duplicate comments (1)
transports/bifrost-http/lib/ctx.go (1)
9-23: Header filter allow/deny checks are still case-sensitive against lowercased headersThe overall structure here (security denylist + configurable allow/deny via
headerFilterConfig+ optional direct forwarding when an allowlist exists) looks good, but the earlier case-sensitivity problem still applies:
- Incoming header names (
keyStr, andlabelNameforx-bf-eh-*) are normalized to lowercase.shouldAllowHeaderand the direct-forwarding block both useslices.Contains(headerFilterConfig.Allowlist, headerName)andslices.Contains(headerFilterConfig.Denylist, headerName)with no normalization of the configured lists.- If the config contains
"Content-Type"while the incoming header is seen as"content-type", it won’t match, so the filter will misbehave (either unintentionally blocking or allowing headers).To make matching truly case-insensitive and predictable you could either:
Normalize config on load (preferred for perf):
- When reading or updating
GlobalHeaderFilterConfigfrom config/DB, lowercase all entries inAllowlistandDenylist.- Then keep using
slices.Containson the already-lowercased lists here.Do case-insensitive membership checks in this function:
- Replace
slices.Containswithslices.ContainsFunc(..., func(s string) bool { return strings.EqualFold(s, headerName) })in bothshouldAllowHeaderand the direct-forwarding block.Also, consider reusing
shouldAllowHeader(keyStr)in the direct-forwarding path instead of reimplementing allow/deny logic there to keep semantics in one place.Also applies to: 80-85, 100-145, 279-297, 298-321
🧹 Nitpick comments (2)
transports/bifrost-http/lib/config.go (1)
1763-1773: Consider adding a nil check for defensive programming.While both current call sites ensure
ConfigStoreis non-nil before calling this function, adding a guard clause inside the function would make it safer if called from new locations in the future.🔎 Proposed fix
func loadDefaultHeaderFilterConfig(ctx context.Context, config *Config) { + if config.ConfigStore == nil { + return + } headerFilterConfig, err := config.ConfigStore.GetHeaderFilterConfig(ctx) if err != nil { logger.Warn("failed to get header filter config from store: %v", err) return } if headerFilterConfig != nil { config.ClientConfig.HeaderFilterConfig = headerFilterConfig } }ui/app/workspace/config/views/clientSettingsView.tsx (1)
305-331: Consider using stable keys for list items.Using
indexas the React key works here since items aren't reordered, but if you add reordering functionality later, you'd need to update this. A simple improvement would be to use the header value itself as the key (since headers should be unique):-{(localConfig.header_filter_config?.allowlist || []).map((header, index) => ( - <div key={index} className="flex items-center gap-2"> +{(localConfig.header_filter_config?.allowlist || []).map((header, index) => ( + <div key={`allowlist-${index}-${header}`} className="flex items-center gap-2">This is a minor improvement and not blocking.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/bifrost-http/integrations/bedrock_test.go
- ui/lib/types/schemas.ts
- framework/configstore/store.go
- transports/bifrost-http/server/server.go
- transports/config.schema.json
- framework/configstore/rdb.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.goui/lib/types/config.tstransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.goui/app/workspace/config/views/clientSettingsView.tsxtransports/bifrost-http/lib/config.goframework/configstore/tables/config.goframework/configstore/clientconfig.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/config.goframework/configstore/tables/config.goframework/configstore/clientconfig.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/config.go
🧬 Code graph analysis (9)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
ui/lib/types/config.ts (1)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)
transports/bifrost-http/integrations/router.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
ui/app/workspace/config/views/clientSettingsView.tsx (2)
ui/lib/types/config.ts (2)
DefaultGlobalHeaderFilterConfig(310-313)GlobalHeaderFilterConfig(304-307)framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)
transports/bifrost-http/lib/config.go (4)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/store.go (1)
ConfigStore(17-161)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
framework/configstore/tables/config.go (2)
ui/lib/types/config.ts (1)
GlobalProxyType(269-269)core/network/http.go (1)
GlobalProxyType(46-46)
framework/configstore/clientconfig.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
🪛 GitHub Actions: PR #1178
transports/bifrost-http/lib/config.go
[error] 40-45: undefined: configstoreTables.GlobalHeaderFilterConfig
🔇 Additional comments (16)
framework/configstore/clientconfig.go (1)
37-51: ClientConfig header filter integration and hashing look solidThe new
HeaderFilterConfigfield and its inclusion inGenerateClientConfigHash(with sorted allowlist/denylist and explicit prefixes) are deterministic and consistent with existing hashing patterns. No issues from this side.Also applies to: 144-171
transports/bifrost-http/handlers/mcp.go (1)
66-68: MCP handler wiring of header filter config is correctPassing
h.store.GetHeaderFilterConfig()intoConvertToBifrostContextwhile keepingallowDirectKeysfalse is consistent with the new API and expected behavior.transports/bifrost-http/handlers/inference.go (1)
449-451: Consistent propagation of header filter config across inference handlersAll inference, batch, and file endpoints now pass both
ShouldAllowDirectKeys()andGetHeaderFilterConfig()intoConvertToBifrostContext, so header filtering applies uniformly without changing existing cancellation/streaming semantics. Looks good.Also applies to: 573-574, 649-650, 731-732, 803-805, 879-880, 1009-1011, 1096-1097, 1395-1396, 1448-1449, 1487-1488, 1526-1527, 1565-1566, 1647-1648, 1706-1707, 1745-1746, 1784-1785, 1823-1824
framework/configstore/tables/config.go (1)
5-13: GlobalHeaderFilterConfig schema is aligned and clearThe new
ConfigHeaderFilterKeyandGlobalHeaderFilterConfig{Allowlist,Denylist}match the documented filtering rules and the TS types. No backend schema issues spotted.Also applies to: 40-49
transports/bifrost-http/handlers/config.go (1)
25-35: Config manager/API exposure for header filter config looks consistentAdding
ReloadHeaderFilterConfigtoConfigManagerand surfacingheader_filter_configfrom the store ingetConfigmirrors how proxy and auth config are handled and keeps the API shape coherent. Just ensure allConfigManagerimplementations are updated to satisfy the new method.Also applies to: 161-167
ui/lib/types/config.ts (1)
268-314: TS types for header_filter_config correctly mirror Go structs
GlobalHeaderFilterConfigand its default value matchtables.GlobalHeaderFilterConfig, and wiring it into bothBifrostConfigandCoreConfigkeeps the UI aligned with the backend schema. No issues from a typing perspective.Also applies to: 321-350
transports/bifrost-http/integrations/router.go (1)
383-404: Generic router now correctly passes header filter config into contextThe updated
ConvertToBifrostContextcall uses bothShouldAllowDirectKeys()andGetHeaderFilterConfig(), so integrations benefit from the same header filtering behavior as the core handlers without altering existing flow control.transports/bifrost-http/lib/config.go (4)
548-559: LGTM!The merge logic for
HeaderFilterConfigcorrectly follows the established pattern: DB values take priority, and empty values are filled from the config file. The handling of both struct-level and list-level merging is well-implemented.
2065-2071: LGTM!The lock-free read pattern is consistent with
ShouldAllowDirectKeys()and the rationale is well-documented. Pointer reads in Go are atomic, making this safe for the intended use case.
500-506: LGTM!The sync logic correctly guards against nil
HeaderFilterConfigand nilConfigStore, and uses an appropriate warning log level for best-effort synchronization failures.
385-389: LGTM!The placement after governance loading and the ConfigStore nil check are appropriate. The comment clarifies that header filter config is runtime-only, which helps explain why it's loaded from the store rather than the config file.
ui/app/workspace/config/views/clientSettingsView.tsx (5)
30-42: Order-sensitive comparison may cause unexpected behavior.The
headerFilterConfigEqualfunction compares arrays by index, meaning["a", "b"]and["b", "a"]are considered different. This could causehasChangesto returntrueeven when the headers are semantically the same but in different order (e.g., if the API returns them in a different order than the user entered).Consider whether order matters for header filtering semantics. If not, you might want a set-based comparison.
🔎 Proposed fix (if order doesn't matter)
function headerFilterConfigEqual(a?: GlobalHeaderFilterConfig, b?: GlobalHeaderFilterConfig): boolean { const aAllowlist = a?.allowlist || []; const bAllowlist = b?.allowlist || []; const aDenylist = a?.denylist || []; const bDenylist = b?.denylist || []; if (aAllowlist.length !== bAllowlist.length || aDenylist.length !== bDenylist.length) { return false; } - return aAllowlist.every((v, i) => v === bAllowlist[i]) && aDenylist.every((v, i) => v === bDenylist[i]); + const aAllowSet = new Set(aAllowlist); + const bAllowSet = new Set(bAllowlist); + const aDenySet = new Set(aDenylist); + const bDenySet = new Set(bDenylist); + + const setsEqual = (a: Set<string>, b: Set<string>) => + a.size === b.size && [...a].every((v) => b.has(v)); + + return setsEqual(aAllowSet, bAllowSet) && setsEqual(aDenySet, bDenySet); }
81-96: LGTM!The cleanup logic correctly filters out empty and whitespace-only entries before saving. The non-null assertion on
bifrostConfigis safe here since the save button is only enabled whenhasChangesis true, which requiresconfigto be defined.
98-157: LGTM!The list handlers are well-implemented with:
- Proper
useCallbackusage with empty dependency arrays- Automatic lowercase conversion for case-insensitive header matching
- Clean immutable state updates
223-293: LGTM!Excellent inline documentation for users. The accordion sections clearly explain:
- How the
x-bf-eh-*prefix works- The relationship between allowlist/denylist and direct header forwarding
- Security-blocked headers that cannot be overridden
This will greatly improve the user experience for configuring header forwarding.
3-28: LGTM!The imports are appropriate for the new header filtering functionality, and the
defaultConfigcorrectly includesDefaultGlobalHeaderFilterConfigas a fallback.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
transports/bifrost-http/lib/ctx.go (1)
80-145: Header filter wiring looks good overall; watch doc drift and case‑sensitivity of config lists
- The new
securityDenylistandshouldAllowHeaderlogic cleanly enforce “always-blocked” headers and the allowlist/denylist precedence, and the direct‑forwarding branch for allowlisted headers is consistent with the UI copy.- The example in the function doc still shows
ConvertToBifrostContext(fastCtx, true)(two args) even though the signature now takesheaderFilterConfigas a third parameter; it’d be good to update that snippet to avoid confusing integrators.- Header names from requests are normalized to lowercase (
keyStr,labelName), but membership checks useslices.Containson the raw allowlist/denylist values. The UI ensures lowercasing, but configs written directly via the API (or older data) can carry mixed‑case header names that will no longer match. Consider either normalizing the config to lowercase once when loading/saving or switching these checks to a case‑insensitive comparison so behavior doesn’t depend on how the config was authored. (This is the same underlying concern noted in the earlier review comment on this file.)Also applies to: 279-321
transports/bifrost-http/lib/config.go (1)
454-507: Centralize and normalize header filter config when loading/syncingThe header filter config flow is coherent:
ClientConfig.HeaderFilterConfigis merged from file/DB, synced to the store viaUpdateHeaderFilterConfig, and reloaded vialoadDefaultHeaderFilterConfig; handlers then read it viaGetHeaderFilterConfig(). One improvement that also resolves the case-sensitivity concern inctx.go:
- Normalize
HeaderFilterConfig.Allowlistand.Denylistto lowercase (and optionally trim/deduplicate) once when loading/syncing here, so downstream lookups against lowercased header names don’t depend on the casing used by the caller (UI vs direct API).For example:
// helper near loadDefaultHeaderFilterConfig func normalizeHeaderFilterConfig(cfg *configstoreTables.GlobalHeaderFilterConfig) { if cfg == nil { return } norm := func(in []string) []string { out := make([]string, 0, len(in)) seen := make(map[string]struct{}, len(in)) for _, h := range in { h = strings.ToLower(strings.TrimSpace(h)) if h == "" { continue } if _, ok := seen[h]; ok { continue } seen[h] = struct{}{} out = append(out, h) } return out } cfg.Allowlist = norm(cfg.Allowlist) cfg.Denylist = norm(cfg.Denylist) }Then call
normalizeHeaderFilterConfiginloadClientConfigFromFilebeforeUpdateHeaderFilterConfig, inloadDefaultHeaderFilterConfigafterGetHeaderFilterConfig, and anywhere else you hydrateClientConfig.HeaderFilterConfig. That keeps the runtime config consistent regardless of how it was authored and makes the existingslices.Containschecks inctx.gorobust. (This aligns with the earlier review note about header filter case-sensitivity.)Also applies to: 548-559, 1561-1580, 1763-1773, 2065-2071
🧹 Nitpick comments (2)
transports/bifrost-http/handlers/inference.go (1)
450-451: Core inference handlers now consistently use headerFilterConfigAll these
ConvertToBifrostContextcall sites (models, text, chat, responses, embeddings, speech, transcription, count_tokens) now passh.config.GetHeaderFilterConfig()alongsideShouldAllowDirectKeys(), so they uniformly honor the global allowlist/denylist while remaining backward compatible when the config is nil. Minor nit: you could also route this viahandlerStore.GetHeaderFilterConfig()for symmetry withShouldAllowDirectKeys(), but the current approach is functionally fine.Also applies to: 573-574, 649-650, 731-732, 803-805, 879-880, 1010-1011, 1096-1097
ui/app/workspace/config/views/clientSettingsView.tsx (1)
3-43: Header forwarding UI and state handling are coherent and match backend semanticsThe new
header_filter_configwiring (defaulting, change detection, trimming empty entries on save, and lowercasing inputs) looks solid and lines up with the server’s expectation of lowercase header names without thex-bf-eh-prefix. One minor nuance:headerFilterConfigEqualtreats the allowlist/denylist as ordered sequences, so reordering entries without changing their content will still count as a change; that’s fine if order isn’t surfaced elsewhere, but you could optionally normalize/sort before comparing if you want purely set‑based semantics.Also applies to: 61-65, 68-81, 98-158, 159-373
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- transports/bifrost-http/handlers/config.go
- ui/lib/types/schemas.ts
- framework/configstore/tables/config.go
- ui/lib/types/config.ts
- transports/config.schema.json
- transports/bifrost-http/integrations/bedrock_test.go
- framework/configstore/clientconfig.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.goui/app/workspace/config/views/clientSettingsView.tsxtransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.goframework/configstore/rdb.gotransports/bifrost-http/lib/config.goframework/configstore/store.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.goframework/configstore/rdb.gotransports/bifrost-http/lib/config.goframework/configstore/store.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.go
🧬 Code graph analysis (8)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
transports/bifrost-http/integrations/router.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
ui/app/workspace/config/views/clientSettingsView.tsx (2)
ui/lib/types/config.ts (2)
DefaultGlobalHeaderFilterConfig(310-313)GlobalHeaderFilterConfig(304-307)framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)
transports/bifrost-http/server/server.go (4)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)transports/bifrost-http/lib/config.go (1)
Config(186-217)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
framework/configstore/rdb.go (2)
framework/configstore/tables/config.go (4)
GlobalHeaderFilterConfig(46-49)TableGovernanceConfig(52-55)TableGovernanceConfig(58-58)ConfigHeaderFilterKey(12-12)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/lib/config.go (3)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
framework/configstore/store.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
🪛 GitHub Actions: PR #1178
transports/bifrost-http/lib/config.go
[error] 40-45: Undefined: configstoreTables.GlobalHeaderFilterConfig
🔇 Additional comments (7)
transports/bifrost-http/handlers/mcp.go (1)
66-68: Propagating headerFilterConfig into MCP context looks correctPassing
h.store.GetHeaderFilterConfig()intoConvertToBifrostContextcleanly aligns MCP tool execution with the new global header filtering behavior; no behavioral regressions are apparent.transports/bifrost-http/handlers/inference.go (1)
1395-1396: Batch and file endpoints correctly wired into global header filter configBatch (create/list/retrieve/cancel/results) and file (upload/list/retrieve/delete/content) handlers now also pass
GetHeaderFilterConfig()intoConvertToBifrostContext, ensuring header forwarding rules are applied consistently across all inference-related APIs.Also applies to: 1448-1449, 1487-1488, 1526-1527, 1565-1566, 1647-1648, 1706-1707, 1745-1746, 1784-1785, 1823-1824
framework/configstore/store.go (1)
126-129: ConfigStore interface extension for header filter config is consistentAdding
GetHeaderFilterConfig/UpdateHeaderFilterConfigunder a dedicated header-filter CRUD section matches existing interface conventions and cleanly exposesGlobalHeaderFilterConfig; assuming all concrete stores implement these, this looks good.transports/bifrost-http/integrations/router.go (1)
392-394: Integration router now passes headerFilterConfig into context conversion
createHandlercorrectly addsg.handlerStore.GetHeaderFilterConfig()to theConvertToBifrostContextcall so provider-specific integrations respect the same global header filtering as the native inference endpoints.transports/bifrost-http/server/server.go (1)
66-67: Header filter reload callback is correctly plumbed and statefulAdding
ReloadHeaderFilterConfigtoServerCallbacksand implementing it by updatings.Config.ClientConfig.HeaderFilterConfig(with a concise allowlist/denylist length log) cleanly wires the persistedGlobalHeaderFilterConfiginto the in-memory config used by HTTP handlers; nil configs are handled safely.Also applies to: 841-856
framework/configstore/rdb.go (1)
1952-1981: Header filter config persistence matches existing governance-config patternsThe get/set helpers are consistent with
GetProxyConfig/UpdateProxyConfig: they upsert a singlegovernance_configrow by key, treat missing/empty values as “no config”, and JSON‑roundtripGlobalHeaderFilterConfig. No issues from a correctness or safety standpoint; normalization of header names (if desired) can be layered in higher-level config code rather than here.transports/bifrost-http/lib/config.go (1)
33-41: The code is correct.GlobalHeaderFilterConfigis properly defined and exported inframework/configstore/tables/config.go(lines 40–49), and the import path and usage inconfig.goare accurate. This type is used consistently throughout the codebase with no compilation errors.
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
8a88ccf to
fb4e862
Compare
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
fb4e862 to
e593d42
Compare
Merge activity
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
transports/bifrost-http/handlers/inference.go (1)
450-451: Header filter config is threaded consistently into all inference contexts.All inference and batch/file handlers now pass
h.config.GetHeaderFilterConfig()intoConvertToBifrostContextalongsideShouldAllowDirectKeys(), which keeps header‑filter semantics uniform across endpoints without changing existing control flow. If this pattern grows further, consider a small helper onCompletionHandlerto DRY the repeated call, but it’s not required for correctness.Also applies to: 573-574, 649-650, 731-732, 803-804, 879-880, 1010-1011, 1096-1097, 1395-1396, 1448-1449, 1487-1488, 1526-1527, 1565-1566, 1647-1648, 1706-1707, 1745-1746, 1784-1785, 1823-1824
transports/bifrost-http/handlers/config.go (1)
25-39: Header filter validation is sound; consider centralizing the security header list.The new plumbing in
ConfigHandlerdoes the right things:
- Rejects changes when
HeaderFilterConfigactually differs, usingheaderFilterConfigEqual.- Validates that neither allowlist nor denylist contain any of the security‑sensitive headers defined in
securityHeaders, case‑insensitively.- Applies the update both to
updatedConfigand viaReloadHeaderFilterConfig, so runtime behavior changes immediately while the config store is still updated viaUpdateClientConfigandReloadClientConfigFromConfigStore.This pairs well with the hardcoded security denylist in
lib/ctx.goand closes the gap where a misconfigured allowlist might otherwise surprise operators.One maintainability concern:
securityHeadershere duplicates the denylist inlib/ctx.go. To avoid future drift, it would be cleaner to centralize this list (for example in a shared internal package or exported helper) and have both the handler and context logic reference the same source.Also applies to: 50-51, 285-299, 648-689
transports/bifrost-http/lib/ctx.go (1)
125-149: LGTM! Case-insensitive header matching properly implemented.The
shouldAllowHeaderclosure correctly implements the filter precedence (allowlist then denylist) with case-insensitive comparison usingstrings.EqualFold. This addresses the case-sensitivity concern from the previous review.Minor nit: extra blank line at line 145 before the closing brace.
🔎 Optional: Remove extra blank line
if slices.ContainsFunc(headerFilterConfig.Denylist, func(s string) bool { return strings.EqualFold(s, headerName) }) { return false - }ui/app/workspace/config/views/clientSettingsView.tsx (2)
53-65: Consider order-insensitive comparison for allowlist/denylist.The current
headerFilterConfigEqualuses index-based comparison, meaning["a", "b"]≠["b", "a"]. While this works for change detection (enabling save), it may causehasChangesto be true even when the effective configuration is identical but in different order.This is a minor UX consideration since header filter semantics are order-independent.
🔎 Optional: Order-insensitive comparison
function headerFilterConfigEqual(a?: GlobalHeaderFilterConfig, b?: GlobalHeaderFilterConfig): boolean { const aAllowlist = new Set(a?.allowlist || []); const bAllowlist = new Set(b?.allowlist || []); const aDenylist = new Set(a?.denylist || []); const bDenylist = new Set(b?.denylist || []); if (aAllowlist.size !== bAllowlist.size || aDenylist.size !== bDenylist.size) { return false; } return [...aAllowlist].every((v) => bAllowlist.has(v)) && [...aDenylist].every((v) => bDenylist.has(v)); }
361-384: Consider using stable keys instead of array index.Using
indexas the React key can cause subtle issues when items are removed (e.g., focus jumping to wrong input). Since header names should be unique within each list, consider using the header value as the key with a fallback:🔎 Optional: Use stable keys
-{(localConfig.header_filter_config?.allowlist || []).map((header, index) => ( - <div key={index} className="flex items-center gap-2"> +{(localConfig.header_filter_config?.allowlist || []).map((header, index) => ( + <div key={header || `empty-${index}`} className="flex items-center gap-2">Apply similarly for the denylist section.
Also applies to: 403-426
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
framework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- framework/configstore/clientconfig.go
- transports/bifrost-http/handlers/mcp.go
- transports/config.schema.json
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/integrations/router.go
- framework/configstore/rdb.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/lib/types/config.tstransports/bifrost-http/handlers/config.gotransports/bifrost-http/server/server.goframework/configstore/tables/clientconfig.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.goui/lib/types/schemas.tsframework/configstore/migrations.gotransports/bifrost-http/integrations/bedrock_test.goui/app/workspace/config/views/clientSettingsView.tsxframework/configstore/tables/config.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/handlers/config.gotransports/bifrost-http/server/server.goframework/configstore/tables/clientconfig.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.goframework/configstore/migrations.gotransports/bifrost-http/integrations/bedrock_test.goframework/configstore/tables/config.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/config.gotransports/bifrost-http/server/server.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/bedrock_test.go
🧬 Code graph analysis (7)
ui/lib/types/config.ts (2)
framework/configstore/tables/config.go (2)
GlobalHeaderFilterConfig(46-49)GlobalProxyConfig(25-38)core/network/http.go (1)
GlobalProxyConfig(55-68)
transports/bifrost-http/server/server.go (4)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)transports/bifrost-http/lib/config.go (1)
Config(186-217)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
framework/configstore/tables/clientconfig.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-409)
transports/bifrost-http/integrations/bedrock_test.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
framework/configstore/tables/config.go (4)
core/providers/gemini/types.go (1)
Type(783-783)ui/lib/types/config.ts (1)
GlobalProxyType(269-269)core/network/http.go (1)
GlobalProxyType(46-46)core/schemas/provider.go (1)
NoProxy(147-147)
🔇 Additional comments (13)
ui/lib/types/config.ts (1)
302-314: Header filter config typing and wiring look consistent with backend.
GlobalHeaderFilterConfig, its default, andCoreConfig.header_filter_config?: GlobalHeaderFilterConfigcleanly mirror the GoGlobalHeaderFilterConfig(allowlist/denylist) and keep the feature optional/backwards‑compatible. No issues from a typing or config‑surface perspective.Also applies to: 322-349
ui/lib/types/schemas.ts (1)
680-691: Zod schemas for header filter config align with TS/Go types.
globalHeaderFilterConfigSchemaand the form wrapper correctly model optional allowlist/denylist arrays, and exported inference types match. Given that security constraints are enforced in the Go handler, keeping this schema simple is appropriate.Also applies to: 692-710
transports/bifrost-http/integrations/bedrock_test.go (1)
9-10: Mock HandlerStore correctly extended with header filter config.The added
headerFilterConfigfield andGetHeaderFilterConfig()method keepmockHandlerStorecompatible with the updatedlib.HandlerStoreinterface while defaulting tonil(no filtering) in existing tests. This should compile and behave as before for current test cases.Also applies to: 17-19, 25-27
framework/configstore/migrations.go (1)
113-115: Header filter config DB migration follows existing patterns and looks safe.The new
migrationAddHeaderFilterConfigJSONColumnis registered intriggerMigrations, conditionally addsheader_filter_config_jsontoTableClientConfig, and cleanly drops it on rollback. It matches the style of other JSON config migrations and should be idempotent across environments.Also applies to: 1834-1866
transports/bifrost-http/server/server.go (1)
54-67: Server reload hook for header filter config is correctly wired.Extending
ServerCallbacksand implementingReloadHeaderFilterConfigby updatings.Config.ClientConfig.HeaderFilterConfigand logging list sizes keeps runtime state aligned with the config store/UI without exposing header names. This matches how proxy and other global configs are handled.Also applies to: 841-856
framework/configstore/tables/config.go (1)
5-13: GlobalHeaderFilterConfig definition matches the intended API surface.
ConfigHeaderFilterKeyandGlobalHeaderFilterConfig{Allowlist,Denylist}cleanly define the new config surface and are consistent with both the UI types and runtime usage inlib/ctx.go. JSON tags and comments document the precedence rules clearly.Also applies to: 40-49
framework/configstore/tables/clientconfig.go (1)
65-73: LGTM! Serialization logic is sound.The choice to serialize
HeaderFilterConfigto empty string""when nil (vs"[]"for slice fields) is appropriate sinceGlobalHeaderFilterConfigis a struct, and empty string cleanly represents "no configuration" semantics. TheAfterFindhook correctly handles this by only deserializing when non-empty.transports/bifrost-http/lib/ctx.go (3)
100-116: LGTM! Security denylist is well-designed.Using a
map[string]boolfor O(1) lookup is efficient. The security-sensitive headers list is comprehensive and correctly includes authentication, connection, and key-related headers that should never be forwarded.
151-156: Debug logging is appropriate.Logging the header filter config at debug level is safe since it only contains header name patterns (not values or secrets). Debug logging is typically disabled in production environments.
310-338: Direct header forwarding logic is well-implemented with defense in depth.The implementation correctly:
- Only activates when allowlist is non-empty (backward compatible)
- Skips
x-bf-*reserved headers- Always enforces security denylist
- Respects user-configured denylist even for allowlisted headers
One consideration: the debug log at line 334 includes the header value. While debug logging is typically off in production, consider whether header values could contain sensitive data (e.g., custom tokens) that shouldn't appear in logs.
ui/app/workspace/config/views/clientSettingsView.tsx (3)
115-136: LGTM! Save logic with security validation and cleanup.Good implementation:
- Defense-in-depth security header check before save
- Empty string filtering ensures clean data persistence
- Non-null assertion on
bifrostConfig!is safe sincehasChangesdepends onconfigbeing defined
271-433: Well-designed UI with comprehensive documentation and security feedback.The Header Forwarding section is well-implemented:
- Clear accordion-based documentation explaining prefixed vs direct headers
- Visual feedback (red border) for invalid security headers
- Consistent RBAC enforcement across all inputs and buttons
- Helpful placeholder examples
32-46: Verify SECURITY_HEADERS consistency with backend.The frontend
SECURITY_HEADERSlist matches the backendsecurityDenylistinctx.go(both contain identical headers in the same order). While this duplication currently maintains consistency, it creates a maintenance burden—if either list is updated without updating the other, the frontend validation will diverge from backend enforcement.
Summary
Add configurable header filtering for extra headers forwarded to LLM providers via the
x-bf-eh-*prefix. This allows administrators to control which headers are allowed to pass through to providers using allowlist and denylist mechanisms.Changes
GlobalHeaderFilterConfigstruct with allowlist and denylist fieldsType of change
Affected areas
How to test
x-bf-eh-*headers to verify filtering works:Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
Checklist
docs/contributing/README.mdand followed the guidelines